-
Notifications
You must be signed in to change notification settings - Fork 164
Fix holdReset glitches for async resets and add registerSyncReset
#3116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| stretchResetAsync clk en SNat rst = rstOut | ||
| where | ||
| -- This ensures the index at least has one cycle for counting. | ||
| counter :: Signal dom (Index ((Max 2 ((Max 1 n) - 1)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dislike this a lot but imo it's better than forcing 3 <= n constraint on the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make it so that you create different circuits for different values of n? i.e.
stretchResetAsync clk en sn@SNat rst = rstOut
where
rstOut = case snatToInteger sn of
0 -> rst
1 -> unsafeToReset (delay clk en isActiveHigh (unsafeFromReset rst))
_ -> unsafeToReset (if isActiveHigh then fmap (/=maxBound) counter else fmap (==maxBound) counter)
counter :: Signal dom (Index n)
counter = register clk rst en 0 (fmap (satSucc SatBound) counter)
isActiveHigh = case resetPolarity @dom of { SActiveHigh -> True; _ -> False }I explicitly left out the resetSynchronizer because the incoming reset should already be synchronously deasserted for the counter to work properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need the reset synchronizer to stop the glitches as indicated by the comment in #3115 (comment)
I thought only one FF is enough though since there should be Recovery and Removal timing checks from the counter output FFs to the reset input. But I'm not 100% sure.
My reasoning for not being 100% sure is that the recovery and removal check only apply to the de-assertion path so timing closure will not ensure any glitch assertion will be safe with respect to recovery and removal checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the linked comment you write assertion, but what you are describing is a glitch deassertion of the outgoing reset. Just FYI; I wondered if you got them mixed up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You just need a simple register on the output to stop the glitching. So in that case rstOut would be something along the lines of:
case snatToInteger sn of
0 -> rst
1 -> unsafeToReset (register clk rst en isActiveHigh (pure (not isActiveHigh))
_ -> unsafeToReset (register clk rst en isActiveHigh (if isActiveHigh then fmap (/=maxBound) counter else fmap (==maxBound) counter)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And even if the outer flop does go meta-stable because CLR/SET port is asserted later on the outer flop than the CLR/SET ports of the counter flops, the circuit will recover, because eventually the assertion will arrive at the CLR/SET port of the outer flop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if you want to use the standard dual FF reset synchronizer, that's fine by me. The only thing I care about is that stretchReset has a simple semantics. If I want the to stretch the deassertion of the reset by 1 cycle, it should just stretch the deassertion by 1 cycle, regardless of whether I have synchronous or asynchronous resets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's important that you can configure it precisely, to control the order in which stuff comes out of reset, but the ability to stretch by very small amounts seems less important (not sure, just can't think of a case where that'd matter much). So if it can't stretch by just 1 or 2 cycles, I think that'd be fine, but if you say n it should really mean n.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes indeed! I now am convinced that this is safe. I was thinking of a possibility where the rst source -> sync FF would be much slower than rst source -> counter FF -> comb logic -> sync FF could induce meta stability. But that's just a standard case with async resets and does not introduce problems since eventually everything will reset. I realized it when I drew it out.
I will update the code and doc. Thank you for the discussion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is solved now. Pleas re-review.
e5e62ad to
9b6053e
Compare
holdReset and add correct implementationholdReset and add correct implementation
9b6053e to
46bccdb
Compare
4be1af9 to
7c377d4
Compare
7c377d4 to
087e8bd
Compare
|
I have removed the
|
7400069 to
f8eeee6
Compare
holdReset and add correct implementationholdReset glitches for async resets and registerSyncReset
3dd15c1 to
3957eac
Compare
|
I ended up keeping the same unregistered implementation for the This also means we don't need a separate implementation or deprecation. @JvWesterveld Are you satisfied with this? |
3957eac to
eb19789
Compare
|
This now again has the weird quirk that for synchronous resets is it holds it for 1 cycle less then one might expect. Although that is something that So maybe this should be a different issue/PR, but the synchronous behavior is something we should document (with a doctest example). |
eb19789 to
a601dac
Compare
425e435 to
1a97376
Compare
holdReset glitches for async resets and registerSyncResetholdReset glitches for async resets and add registerSyncReset
b3898f4 to
7aef9ce
Compare
While I don't like that behaviour, I would like to fix We could fix that quirk in 1.10, but if we just fix @rowanG077 Could you please label this PR with |
JvWesterveld
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rowanG077 Yes, approach LGTM. Thank you for picking it up.
0bc273e to
7d781b7
Compare
7d781b7 to
c8ad98c
Compare
leonschoorl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With that orReset it fixes the weird quirk for synchronous resets, which is very nice!
But since this now creates a (small) behavioral change compared to the previous behavior for synchronous resets, should we be be backporting this to 1.8?
|
@leonschoorl We talked about this in the office and in chat. Since the old behavior is considered broken we think it should be backported. |
for async resets add `registerSyncReset`
c8ad98c to
a9a880c
Compare
|
I didn't get any green review checkmarks yet. But all issues have been resolved. So if anyone has any problems with this PR please let me know. |
for async resets add `registerSyncReset` (cherry picked from commit b1b1477) # Conflicts: # clash-prelude/src/Clash/Signal.hs # tests/shouldwork/Signal/ResetGen.hs
for async resets add `registerSyncReset` (cherry picked from commit b1b1477) # Conflicts: # clash-prelude/src/Clash/Signal.hs # tests/shouldwork/Signal/ResetGen.hs
for async resets add `registerSyncReset` (cherry picked from commit b1b1477) # Conflicts: # clash-prelude/src/Clash/Signal.hs # tests/shouldwork/Signal/ResetGen.hs
for async resets add `registerSyncReset` (cherry picked from commit b1b1477) # Conflicts: # clash-prelude/src/Clash/Signal.hs # tests/shouldwork/Signal/ResetGen.hs Co-authored-by: Rowan Goemans <goemansrowan@gmail.com>
Aimed to address #3115
This updates the documentation of
holdResetto indicate the problems it has. It also gives a new functionstretchResetthat should have the correct behaviour but has slightly different timing characteristics so it's not a drop in replacement.I have marked the
holdResetfunction with a warning, I don't like it since now we introduce warning into our own project when exposing it and using it in tests. I also thought a deprecation pragma. Both have some implication as that requires a version bump according to haskell PVP specification.Still TODO: